Conversation
| ZVAL_DEREF(data); | ||
| if (Z_TYPE_P(data) != IS_STRING) { | ||
| zend_throw_exception_ex(phar_ce_PharException, 0, "Non-string value passed to Phar::mungServer(), expecting an array of any of these strings: PHP_SELF, REQUEST_URI, SCRIPT_FILENAME, SCRIPT_NAME"); | ||
| zend_argument_type_error(1, "must be an array of string types, %s given", zend_zval_value_name(data)); |
There was a problem hiding this comment.
To be pedantic, array of strings? It doesn't contain types.
There was a problem hiding this comment.
Being pedantic is good :) Will amend to use this error message
|
|
||
| if ((index || webindex) && (phar_obj->archive->is_tar || phar_obj->archive->is_zip)) { | ||
| zend_argument_value_error(index ? 1 : 2, "must be null for a tar- or zip-based phar stub, string given"); | ||
| zend_argument_type_error(index ? 1 : 2, "must be null for a tar- or zip-based phar stub, string given"); |
There was a problem hiding this comment.
This one seems a bit unnecessary. I think type errors mostly apply to signatures, whereas this is a logical issue.
|
|
||
| php_stream_close(fp); | ||
| if (files_ht && UNEXPECTED(zend_hash_num_elements(files_ht) == 0)) { | ||
| zend_argument_must_not_be_empty_error(2); |
| /* TODO: This error message is nonsensical */ | ||
| zend_throw_exception_ex(spl_ce_InvalidArgumentException, 0, | ||
| "Invalid argument, extraction path must be non-zero length"); | ||
| "Invalid argument, %s cannot be found", phar_obj->archive->fname); |
There was a problem hiding this comment.
Maybe "opened" instead of "found", but I don't know all the reasons for which this can fail.
There was a problem hiding this comment.
The thing is that it is the filename of the Phar archive, and I'm struggling to see how this condition could ever be reached. Although it seems we have test coverage of it according to CodeCov as ext/phar/tests/phar_extract3.phpt is triggering the code path.
It seems that PharData doesn't check if the filename given does actually exist until something is performed with it :/
| if (IS_STRING != Z_TYPE_P(zval_file)) { | ||
| zend_throw_exception_ex(spl_ce_InvalidArgumentException, 0, | ||
| "Invalid argument, array of filenames to extract contains non-string value"); | ||
| zend_argument_type_error(2, "must be an array of string elements, %s given", zend_zval_value_name(zval_file)); |
There was a problem hiding this comment.
Inconsistent with the above, maybe just "array of strings".
Main purpose is to use ValueError/TypeError in cases that are about value/type validation. This fixes the wording to match the more usual wording. Secondly use Error when an object is not initialized or one attemps to call the constructor twice, similar to other extensions
TODO: It seems like one needs to copy the files from ext/phar/tests/files/ to ext/phar/tests/cache_list/files/ and that they were out of sync
Girgias
force-pushed
the
2026-04-phar-reclassify-exceptions
branch
from
c6e0e04 to
011c4d0
Compare
| code = Z_LVAL_P(val); | ||
| } else { | ||
| zend_throw_exception_ex(phar_ce_PharException, 0, "Unknown mime type specifier used, only Phar::PHP, Phar::PHPS and a mime type string are allowed"); | ||
| zend_argument_value_error(4, "mime type specifier must be one of Phar::PHP or Phar::PHPS"); |
There was a problem hiding this comment.
this isn't really accurate since it can also be a string, maybe "must be one of Phar::PHP or Phar::PHPS when using an integer"?
| if (!value) { | ||
| /* failure in get_current_data */ | ||
| zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0, "Iterator %s returned no value", ZSTR_VAL(ce->name)); | ||
| /* This should never happen without an exception */ |
There was a problem hiding this comment.
I'm a bit confused by this comment - if it indicates that there should have already been an exception, should we ZEND_ASSERT(EG(exception)); ?
If this indicates that we should now trigger an exception, then I guess I'm wondering why the comment is needed - maybe clarify "Failures in get_current_data trigger exceptions here"?
| if (IS_STRING != Z_TYPE_P(zval_file)) { | ||
| zend_throw_exception_ex(spl_ce_InvalidArgumentException, 0, | ||
| "Invalid argument, array of filenames to extract contains non-string value"); | ||
| zend_argument_type_error(2, "must be an array of strings, %s given", zend_zval_value_name(zval_file)); |
There was a problem hiding this comment.
this is a bit confusing - if the value is an array of things, one of which is a non-string, lets say into, instead of saying "array containing an int given" given it'll say "int given", but what was actually given was an array
Maybe
"must be an array of only strings, array containing %s given"
?
3 participants
Main purpose is to use ValueError/TypeError in cases that are about value/type validation. This fixes the wording to match the more usual wording.
Secondly use Error when an object is not initialized or one attemps to call the constructor twice, similar to other extensions
We also fix some of the phar test stubs. Which we regenerate by running the
.phar.incfile.